fix(process): reap entire process group on cmd.Cancel - #39143

Please trim down on comments.

What do you think about this bc21fdd ?

@wxiaoguang while a much bigger refactor, your change will be safer because mine relied on callers to use setSysProcAttribute(), whereas now it would be the default behavior. The move to SIGTERM is also preferable, however I believe it introduced a new bug: if the child ignores SIGTERM it could hang forever, and grandchildren would never get a signal.

I've added a few test cases to prove this, and my proposed fix is just to introduce a default 10s (configurable) wait on Cancel() before escalating to SIGKILL: 9e336b1

however I believe it introduced a new bug: if the child ignores SIGTERM it could hang forever, and grandchildren would never get a signal.

I've added a few test cases to prove this, and my proposed fix is just to introduce a default 10s (configurable) wait on Cancel() before escalating to SIGKILL: 9e336b1

I intentionally didn't do that because it will just cause pid data-race problems. You can ask AI about the PID-race details.

So I strongly prefer to revert 9e336b1 (Actually, I mean, it needs to be reverted)

And, I don't see why the SIGTERM would be ignored in real world for our cases. If it would happen, I already prepared WithOnCancelForceKill

pid data-race problems.

Good point, although technically SIGINT suffers from this as well.

I don't see why the SIGTERM would be ignored in real world for our cases

Maybe not 'ignored' in real-world scenarios, but certainly blocked longer than what we might be willing to wait. How long is too-long is really only known by the caller, but I could see scenarios where that external renderer or ssh call sites hang on some connection. Is 5 minutes too-long? Is 30 seconds?

It looks like ProcessState can be trusted while we hold a reference, how about we just check it before doing any syscall? 3b7449f

I don't see why the SIGTERM would be ignored in real world for our cases

Maybe not 'ignored' in real-world scenarios, but certainly blocked longer than what we might be willing to wait. How long is too-long is really only known by the caller, but I could see scenarios where that external renderer or ssh call sites hang on some connection. Is 5 minutes too-long? Is 30 seconds?

It looks like ProcessState can be trusted while we hold a reference, how about we just check it before doing any syscall? 3b7449f

I don't see it would really affect real world use cases. And there is no easy solution for the PID reuse data-race.

I believe we can take the graceful kill (SIGTERM) as default behavior, if anything wrong, just switch to SIGKILL.

It looks like ProcessState can be trusted while we hold a reference, how about we just check it before doing any syscall? 3b7449f

No, it's not right. "there is no easy solution for the PID reuse data-race."

I don't see it would really affect real world use cases

I think the real-world use-case is pretty theoretical, for sure, but this would reduce the window of pid reuse from however long since the process was started, to probably nanoseconds.

I don't see it would really affect real world use cases

I think the real-world use-case is pretty theoretical, for sure, but this would reduce the window of pid reuse from however long since the process was started, to probably nanoseconds.

Let's just try to use SIGTERM as the default signal in production to see whether there would be real problems. If yes, just switch to SIGKILL.

FYI that this may need a follow-up, it was decided not to use SIGKILL, and with SIGINT this still leaves child processes like pack-objects --revs --thin --stdout --progress --delta-base-offset running on gitea. The way I replicated this was to execute a git clone, then SIGINT my git clone process. You can see on gitea it spawning git-remote-http (in my case, maybe with ssh this bug doesn't exist?) and the children. The git-remote-http disappears after killing the client's git-clone process, but the children still persist.

FYI that this may need a follow-up, it was decided not to use SIGKILL, and with SIGINT this still leaves child processes like pack-objects --revs --thin --stdout --progress --delta-base-offset running on gitea.

Can these processes be killed by SIGTERM?

• If yes: need to figure why they are not killed, because we already sent SIGTERM to their process groups
• If no: then we have to use SIGKILL (I doubt about it, see below, pack-objects process can be killed gracefully)

You can see on gitea it spawning git-remote-http (in my case, maybe with ssh this bug doesn't exist?) and the children.

Are you sure git-remote-http is spawned by Gitea?

Some details are still unclear to me because it seems that the problem you described is not that what should happen.

Two AIs say: git-remote-http runs on the client side.

Details

Also, I have verified, git pack-objects can be killed by SIGTERM when it is running.

You can try it by: run git pack-objects HEAD in one terminal, then run kill ... in another terminal.

So, I don't think the git pack-objects processes you saw could be related to the SIGTERM / SIGKILL (suppose the new code has sent SIGTERM to their process groups)

Also, you can set c.termGraceful = false in process.CommandContext, then it should be exactly the same as the first commit in this PR 8686ec5 : syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL), to see whether there would be differences.